Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: v8 version 11.1.277.17 #219

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

feat: v8 version 11.1.277.17 #219

wants to merge 38 commits into from

Conversation

NathanWalker
Copy link
Contributor

No description provided.

caitp added 30 commits June 20, 2023 14:33
This does not incorporate catalyst libraries.
FIXME: Remove this the handle visitor class entirely if we can't use it anymore.
Make it a bit easier to identify what object is being allocated by MakeGarbageCollected
while logging
…fields or v8::Externals

Mostly a debugging tool, we can revert this
Previously extended class constructors stored their CacheItem in a single
InternalField, and the CacheItem was retained by the Caches collection for the
Isolate.

Consider adding a separate wrapper type, or a flag to the ObjCDataWrapper struct,
to indicate the disposal semantics --- However so far I don't think this is an issue.
I'm not entirely positive this is actually used anywhere, could not find it in the diff
This reverts commit d5d9f30.

This change was not actually used by anything in the staging area
This change replaces the v8 root/persistent handle in the ReferenceWrapper with a traced value, which should allow it to be collected more easily, and fit in better with the general approach we're following here. It does still use a nullable heap-allocated pointer to a TracedValue, in order to have somewhat consistent behaviour with the previous version.
This is a minor change, mostly involving refactors. The gist of it is, Pointers are GC'd structures,
and the update allows them to be traced correctly. It is in some ways a bit cleaner, but should be
semantiaclly equivalent to the original version. There is a downside in that the result of the Pointer()
constructor will now be slightly larger than previously.
The mixup between having a wrappable block/function type managed by CPPGC, while
also using referencing the value with a v8::External associated with the function,
caused a bit of an issue.

This is an area where potentially using GC-able wrappable types is an error, as
v8 currently does not make any effort to keep the value alive via the FunctionTemplate's
data slot. However, given that the JS value should live as long as the API function, for now
it may be a moot point.

We should revisit this later.
This is one of the larger changes to the codebase --- As usual, there is a refactoring
related to things such as the use of v8::Externals, which are now replaced by JSObjects
with 2 embedder slots suitable for holding CPPGC values and tracing them effectively.

The old approach to retaining the "parent" struct of a StructWrapper is simplified to
rely on a traced value, avoiding the need for shared pointer tracking.
Following the usual pattern of cleanup and replacing v8::Externals and explicit lifetime management
with GarbageCollected CPPGC types and JS wrapper objects.
Following a similar pattern to other changes.
Because CPPGC grants us the ability to handle finalization of script wrpapable types in a GarbageCollected<T>'s destructor,
this allows us to greatly simplify how this works.

However, currently there are a lot of interactions between the instance caching subsystem and object manager, and there are
tests which verify this behaviour.

For its part, __releaseNativeCounterpart() should still behave somewhat consistently with how it did, in its previous incarnation,
although it likely is buggy to some degree, particularly if native data is shared between multiple threads.

It's a work in progress to fix this and further simplify it. However, we do need to come up with a more sensible mechanism for
properly retaining native data, and cleaning up JS wrappers.
@cla-bot cla-bot bot added the cla: yes label Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants